Reviewed 10 of 10 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @srfrog)

dgraph/cmd/alpha/admin_backup.go, line 54 at r1 (raw file):

		since, err = strconv.ParseUint(at, 10, 64)
		if err != nil {
			x.SetStatus(w,

Does this actually check that since is greater than zero? 0 is still a valid uint so it will be converted just fine

ee/backup/file_handler.go, line 98 at r1 (raw file):

		if err != nil {
			if glog.V(2) {
				fmt.Printf("--- Skip: invalid backup name format: %q\n", file)

Why the "---"? Do any existing log lines have them? Otherwise it might just be better to remove them.

ee/backup/handler.go, line 33 at r1 (raw file):

	Create(*url.URL, *Request) error
	// Load will scan location URI for backup files, then load them with loadFunc.
	Load(*url.URL, uint64, loadFn) error

is it possible to give names to the arguments in these functions? I think it would be a bit more readable.

worker/backup_ee.go, line 95 at r1 (raw file):

// BackupOverNetwork handles a request coming from an HTTP client.
func BackupOverNetwork(ctx context.Context, destination string, since uint64) error {
	ctx, cancel := context.WithCancel(ctx)

is it fine to overwrite ctx here?

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @martinmr)

dgraph/cmd/alpha/admin_backup.go, line 54 at r1 (raw file):

If we get an error here it means the value was not a proper uint (e.g., at=horse or at=-10) . If at= is 0 then none of the logic is trigged so it wont matter. And I don't check values attached to errors, ever.

ee/backup/file_handler.go, line 98 at r1 (raw file):

Logs can get noisy, the dashes mean "while doing what I said above just now, this happened...". Since it's a printf to stdout, there are no visual artifacts to follow the trail. Imagine 100 files, 1000, etc.

ee/backup/handler.go, line 33 at r1 (raw file):

This is an interface not a function definition. The Go standard/idiomatic-way is not name the abstract types. The actual function definition (concrete type) will have the arg names and how it uses them. But I will expand the comments which is really what explains how these might work.

worker/backup_ee.go, line 95 at r1 (raw file):

Yes, a new context is created using the incoming as parent. And the parent is a background context which is a static reference.

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @martinmr)

ee/backup/file_handler.go, line 98 at r1 (raw file):

Why not use glog here? It's only printed when glog.V(2), but it doesn't log with glog?

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @martinmr)

ee/backup/file_handler.go, line 98 at r1 (raw file):

restore only works in CLI. glog looks ugly because it has timestamp when the rest doesn't

Reviewed 4 of 9 files at r7, 2 of 7 files at r8, 5 of 5 files at r11.
Reviewable status: 14 of 15 files reviewed, 17 unresolved discussions (waiting on @gitlw, @manishrjain, @martinmr, and @srfrog)

ee/backup/backup_test.go, line 52 at r11 (raw file):

			conn, err := grpc.Dial("localhost:9180", grpc.WithInsecure())
			x.Check(err)
			dg := dgo.NewDgraphClient(api.NewDgraphClient(conn))

FYI, the z.DgraphClientNoDropAll() does exactly this.

ee/backup/backup_test.go, line 86 at r11 (raw file):

	}
	for i := range dirs {
		x.Check(os.RemoveAll(dirs[i]))

You can't just remove the whole data directory instead of subdirs individually? Are there additional files there?

Reviewable status: 14 of 15 files reviewed, 17 unresolved discussions (waiting on @codexnull, @gitlw, @manishrjain, and @martinmr)

ee/backup/backup_test.go, line 86 at r11 (raw file):

I thought so too, but docker compose had issues starting without the dir being there. Maybe I'm missing something. So I left data/ with a gitkeep.

Reviewed 1 of 9 files at r7, 10 of 10 files at r12.
Reviewable status: all files reviewed, 21 unresolved discussions (waiting on @gitlw, @manishrjain, @martinmr, and @srfrog)

ee/backup/run.go, line 119 at r12 (raw file):

			grpc.WithBlock(),
			grpc.WithInsecure(),
			grpc.WithTimeout(10*time.Second))

grpc.WithTimeout() deprecated according to the docs, so it's probably best to not use it in new code. Can you use context.WithTimeout() here like you do a few lines below?

ee/backup/s3_handler.go, line 222 at r12 (raw file):

	}

	const N = 100

How about a better name for this variable? BATCH_SIZE, MAX_PENDING, ...

ee/backup/s3_handler.go, line 235 at r12 (raw file):

			}(i, o)
			if i%N == 0 {
				wg.Wait()

Won't this wait after the first object (i == 0, and 0 % anything == 0)? I think you want (i+1)%N instead.

ee/backup/s3_handler.go, line 237 at r12 (raw file):

				wg.Wait()
			}
		}

Does this loop have a potential deadlock or do I not understand how channels work?

Suppose there are 101 objects. The loop will start all goroutines (i = 0 through 100) and then wait for them to finish. But the rc channel has a buffer length of 1, so 100 of them will block until the rc channel is read, which won't happen because the parent routine is in wg.Wait().

Should the make(chan) have a buffer length of N?

ee/backup/s3_handler.go, line 258 at r12 (raw file):

	defer close(doneCh)

	suffix := "/" + backupManifest

Can you use filepath.Join() here as well?

Reviewable status: all files reviewed, 21 unresolved discussions (waiting on @codexnull, @gitlw, @manishrjain, and @martinmr)

ee/backup/run.go, line 119 at r12 (raw file):

Done.

ee/backup/s3_handler.go, line 123 at r8 (raw file):

Done.

ee/backup/s3_handler.go, line 222 at r12 (raw file):

Done.

ee/backup/s3_handler.go, line 235 at r12 (raw file):

nice catch, fixed

ee/backup/s3_handler.go, line 237 at r12 (raw file):

Yes you are correct, I had moved wg.Wait() outside, that needed to be a time.Sleep() and a larger buffer like you suggested.

ee/backup/s3_handler.go, line 258 at r12 (raw file):

I could but this is an URI path which only uses /. I use filepath.Join() in the file handler in case it is an NTFS mount or Dgraph is used on Windows.

All good for now. I'll work a bit on backups once you merge.

Reviewable status: 15 of 16 files reviewed, 15 unresolved discussions (waiting on @codexnull, @gitlw, @manishrjain, and @martinmr)

ee/backup/run.go, line 119 at r12 (raw file):

grpc.WithTimeout() deprecated according to the docs, so it's probably best to not use it in new code. Can you use context.WithTimeout() here like you do a f

This comment hasn't been addressed, but marked as done.